Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate ref.setNativeProps in favor of ReactNative.setNativeProps #14912

Merged
merged 5 commits into from
Feb 25, 2019

Conversation

elicwhite
Copy link
Member

Related to: react-native-community/discussions-and-proposals#72

We want people to call the new top level API for setNativeProps that was added in #14907. This PR adds deprecation warnings for the current methods.

I need to think about how I want to handle the migration of core and internal and could use some insight. I could:

  • hold off on landing this and instead update core to the new API before any warnings land
  • land this and add a yellow box ignore internally as we are converting core. If this change goes out with 0.60 then we have time. We might want to land this in 0.59 to have the warnings out as soon as possible although that would also mean we'd need to cherry pick the fixes for these warnings in core into 0.59

@cpojer
Copy link
Contributor

cpojer commented Feb 21, 2019

How long will it take to convert all of RN Core to use the new API? I feel like adding a warning for everyone in the community and silencing them internally isn't great :(

@sizebot
Copy link

sizebot commented Feb 21, 2019

Details of bundled changes.

Comparing: f978d5f...f863070

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.2% 610.01 KB 611.41 KB 130.95 KB 131.17 KB RN_FB_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 246.95 KB 246.95 KB 43.1 KB 43.1 KB RN_FB_PROD
ReactNativeRenderer-profiling.js 0.0% 0.0% 253.3 KB 253.3 KB 44.64 KB 44.65 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.2% +0.2% 609.93 KB 611.32 KB 130.91 KB 131.14 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 246.96 KB 246.96 KB 43.1 KB 43.1 KB RN_OSS_PROD
ReactFabric-dev.js +0.2% +0.2% 600.87 KB 602.26 KB 128.68 KB 128.89 KB RN_FB_DEV
ReactFabric-prod.js 0.0% -0.0% 239.3 KB 239.3 KB 41.62 KB 41.62 KB RN_FB_PROD
ReactFabric-dev.js +0.2% +0.2% 600.77 KB 602.17 KB 128.63 KB 128.84 KB RN_OSS_DEV
ReactFabric-prod.js 0.0% -0.0% 239.3 KB 239.3 KB 41.61 KB 41.61 KB RN_OSS_PROD

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js 0.0% 0.0% 23.32 KB 23.32 KB 6.04 KB 6.04 KB NODE_DEV
scheduler-tracing.development.js +1.7% +4.2% 10.32 KB 10.49 KB 2.38 KB 2.48 KB NODE_DEV
scheduler-tracing.production.min.js 0.0% 🔺+0.3% 728 B 728 B 382 B 383 B NODE_PROD
scheduler-tracing.profiling.min.js 0.0% -0.1% 3.26 KB 3.26 KB 1000 B 999 B NODE_PROFILING
SchedulerTracing-dev.js +0.8% +0.8% 9.88 KB 9.97 KB 2.07 KB 2.08 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@elicwhite
Copy link
Member Author

Well, just loading the internal Playground view with this change popped up 115 warnings. Sounds like we should migrate the core before landing this.

@elicwhite
Copy link
Member Author

I'm thinking that I will probably want to land this and add the warning to the yellowbox ignores for core RN so that nobody sees the error but it will be easy for us to comment out the ignore line and fix them. Right now in order to see these warnings we have to do a sync first which is painful.

@sebmarkbage
Copy link
Collaborator

We do have separate builds from FB and open source. If you want, you could add this behind a feature flag that we only enable for FB.

Add it here: https://github.com/facebook/react/blob/master/packages/shared/ReactFeatureFlags.js

Enable here: https://github.com/facebook/react/blob/master/scripts/rollup/shims/react-native-fb/ReactFeatureFlags.js

@@ -27,6 +27,7 @@ export const debugRenderPhaseSideEffectsForStrictMode = true;
export const disableInputAttributeSyncing = false;
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
export const warnAboutDeprecatedLifecycles = true;
export const warnAboutDeprecatedSetNativeProps = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Is this the canonical one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by canonical? It seems like to get Flow to pass these consts have to be defined in every file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nvm. I understand. I need to re-export the one from FeatureFlags. I'm testing this change more closely to ensure I can actually flip this on in FBSource.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should actually remove it from the dynamic one if possible. Whenever possible we should always use static flags and drive to remove the dynamic ones.

@elicwhite
Copy link
Member Author

Verified that these warnings are now on in FB:
simulator screen shot - iphone xs - 2019-02-25 at 12 32 36

@elicwhite elicwhite merged commit 870214f into facebook:master Feb 25, 2019
@elicwhite elicwhite deleted the setnativeprops-warning-with branch February 25, 2019 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants